Fix importRoot by casio · Pull Request #69 · suitcss/preprocessor

casio · 2016-12-01T08:27:10Z

currently this fixes #64

however, there's still ambiguity with the CLI flag importRoot and root coming from config files.
importRoot takes precedence now, but I guess both should just be named root.

also, we might consider output to be resolved around the same root. having thought about that a little more, I think we actually should not - most of the time the output will go into some arbitrarily located dist folder, I guess.

thoughts?

simonsmith

Thanks for putting this together @casio

I think having program.importRoot and config.root is okay. It allows us to release this change as a patch and seeing as you've set importRoot to take precedence over root that matches the expected behaviour.

I agree that output should be left alone.

simonsmith · 2016-12-01T10:38:07Z

bin/suitcss

+if (input) {
+  var resolvedInput = importRoot ? resolve(importRoot, input) : resolve(input);
+  if (!exists(resolvedInput)) logger.fatal('not found', resolvedInput);
+  else input = resolvedInput;


Could we use braces here for the if/else? Just matches it up with everything else

giuseppeg · 2016-12-01T10:56:22Z

test/cli.js

@@ -59,15 +59,15 @@ describe('cli', function() {
  });

  it('should output stylelint warnings', function(done) {


these are testing other feature, would you mind creating a suite only to test the importRoot thing instead of mixing tests up?

hm, test/cli.js currently is really more of an integration test suite, no?
so imho its ok to test all i/o stuff there.
it would probably be cleaner to eg remove the -i options out of unrelated cases, though

I mean a nested describe in test/cli.js with the importRoot relevant tests.
The point is testing imports indirectly i.e. by tweaking other tests to cover some use cases it is not optimal.

giuseppeg · 2016-12-01T10:58:03Z

should we also fix this part?

simonsmith · 2016-12-01T11:11:33Z

@giuseppeg Do you mean normalising it so it always refers to root?

giuseppeg · 2016-12-01T14:37:20Z

@simonsmith we said that CLI flags should always override config file options, in this case --importRoot won't because we check only for program['root'], we should add importRoot to the array.

Do you mean normalising it so it always refers to root?

Basically yes

simonsmith · 2016-12-04T14:25:25Z

Maybe get this merged and we can address the other issue separately. Do we just need to make those test case changes?

giuseppeg · 2016-12-05T10:54:20Z

Do we just need to make those test case changes?

Yeah.
I can fix the other part in a follow-up branch if you prefer.

simonsmith · 2016-12-05T20:30:40Z

Might be easier as it can be done in parallel. Then I think we're set to release a patch

casio · 2016-12-06T11:45:21Z

hey guys! sorry, have been a bit busy & thus silent. please ping me if you'd like me to add more changes or squash the prev commits.

simonsmith · 2016-12-06T14:25:32Z

@casio Not a problem, we're all busy at times :)

I think if you could address the feedback about grouping the tests in a single describe then we're good to go

Relates to #69

casio · 2016-12-07T11:54:39Z

so I just put the importRoot related test in its own nested describe block, as @giuseppeg suggested.
also I only checked for errors here(as @simonsmith did in his current pr).

it might be a good idea to, in one of the next steps, switch tests unrelated to importRoot to not make use if -i at all, but I thought that'd be better to do in another pr.

simonsmith · 2016-12-07T11:59:54Z

@casio Yes, agreed. We can just give the full path to the fixtures which hilariously it seems like I had done even with -i which hid this bug in the first place :D

casio · 2016-12-07T12:17:59Z

@simonsmith haha : )

fyi seeing appveyor bark, I just pushed an update. strangely the local lint task didnt catch that(done in describe call & a missing semicolon).

simonsmith · 2016-12-07T12:56:36Z

I think this looks good to merge. @giuseppeg ?

giuseppeg · 2016-12-07T13:25:58Z

@casio were the other tests failing after your changes? I think that importRoot shouldn't affect the input file path @simonsmith correct me if I am wrong.

casio · 2016-12-07T13:49:49Z

@giuseppeg yes, they were. actually with how this is currently implemented, the name inputRoot would better mimic the behaviour.

and you're right, I guess: basically these are two different things - a root for the input(=entry) file and another one to resolve @import statements.

maybe it'd even be beneficial to revisit the concept/scope of how these things should work: ie. should it be rather import roots(plural), do we need a separate base path to resolve input and output, etc.
however I feel that'd go beyond the scope of this pr?

simonsmith · 2016-12-07T14:41:20Z

I don't see any failing tests?

casio · 2016-12-07T14:44:28Z

I think @giuseppeg meant tests that read like
... -i path/to/fixtures path/to/fixtures/input.css

Theses were failing with the new behavior since the cli tried to read
path/to/fixtures/path/to/fixtures/input.css then.

giuseppeg · 2016-12-07T15:23:02Z

Yea imporRoot is about @imports not input files.
We should fix this, please also revert the changes to the old tests.

I can take a look at the issue again and the PR, I am just hopping on and off and I haven't been able to read it carefully because I am busy – sorry about that :)

simonsmith · 2016-12-07T15:24:40Z

Interesting, I see what you guys mean.

Yeah it should relate directly to the postcss import root option.

giuseppeg · 2016-12-07T15:27:00Z

Took a quick look, this is a non-issue because --import-root wasn't meant for this purpose i.e. pass input files without basepath. We should only fix #70 I suppose.

simonsmith · 2016-12-07T15:29:44Z

So are we saying the issue in #64 is expected?

Should the user be prepending the correct path to the input file?

giuseppeg · 2016-12-07T15:32:45Z

It has always been like that. Ideally the root can be different than the input file's folder.

simonsmith · 2016-12-07T15:35:23Z

I see. I guess that does correctly mimic how it would with postcss-import directly. You'd pass a full path to an input file and also be able to set a separate import root if needed. It is somewhat confusing but I think in most cases you rarely need to set -i.

simonsmith · 2016-12-08T11:35:34Z

So it looks like this is PR is not worth merging then? I fear we may have led you down a wrong path @casio. My apologies if so D:

casio · 2016-12-08T12:24:32Z

@simonsmith haha, no worries. was worth the ride : )

giuseppeg · 2016-12-08T12:30:47Z

thank you for your time @casio! Sorry for the inconvenience.

Relates to #69

simonsmith requested changes Dec 1, 2016

View reviewed changes

giuseppeg reviewed Dec 1, 2016

View reviewed changes

casio force-pushed the fix/import-root branch from df11fdf to 7431055 Compare December 1, 2016 11:03

simonsmith approved these changes Dec 4, 2016

View reviewed changes

simonsmith added a commit that referenced this pull request Dec 6, 2016

Allow --importRoot to override root config option

e9617b4

Relates to #69

simonsmith added a commit that referenced this pull request Dec 6, 2016

Allow --importRoot to override root config option

d85959d

Relates to #69

simonsmith mentioned this pull request Dec 6, 2016

Allow --importRoot to override root config option #70

Merged

casio force-pushed the fix/import-root branch from 7431055 to 2dfbf78 Compare December 7, 2016 11:52

Resolve input files relative to importRoot(or config.root)

98ddec0

casio force-pushed the fix/import-root branch from 2dfbf78 to 98ddec0 Compare December 7, 2016 12:10

casio closed this Dec 8, 2016

simonsmith added a commit that referenced this pull request Dec 8, 2016

Allow --importRoot to override root config option

ce0a8b9

Relates to #69

giuseppeg mentioned this pull request Dec 9, 2016

--import-root is being ignored #64

Closed

		@@ -59,15 +59,15 @@ describe('cli', function() {
		});

		it('should output stylelint warnings', function(done) {

Conversation

casio commented Dec 1, 2016

Uh oh!

simonsmith left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonsmith Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

casio Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

giuseppeg Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

casio Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

giuseppeg Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppeg commented Dec 1, 2016

Uh oh!

simonsmith commented Dec 1, 2016

Uh oh!

giuseppeg commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonsmith commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppeg commented Dec 5, 2016

Uh oh!

simonsmith commented Dec 5, 2016

Uh oh!

casio commented Dec 6, 2016

Uh oh!

simonsmith commented Dec 6, 2016

Uh oh!

casio commented Dec 7, 2016

Uh oh!

simonsmith commented Dec 7, 2016

Uh oh!

casio commented Dec 7, 2016

Uh oh!

simonsmith commented Dec 7, 2016

Uh oh!

giuseppeg commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casio commented Dec 7, 2016

Uh oh!

simonsmith commented Dec 7, 2016

Uh oh!

casio commented Dec 7, 2016

Uh oh!

giuseppeg commented Dec 7, 2016

Uh oh!

simonsmith commented Dec 7, 2016

Uh oh!

giuseppeg commented Dec 7, 2016

Uh oh!

simonsmith commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppeg commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonsmith commented Dec 7, 2016

Uh oh!

simonsmith commented Dec 8, 2016

Uh oh!

casio commented Dec 8, 2016

Uh oh!

giuseppeg commented Dec 8, 2016

Uh oh!

Reviewers

Assignees

simonsmith left a comment •

edited

Loading

giuseppeg Dec 1, 2016 •

edited

Loading

giuseppeg commented Dec 1, 2016 •

edited

Loading

simonsmith commented Dec 4, 2016 •

edited

Loading

giuseppeg commented Dec 7, 2016 •

edited

Loading

simonsmith commented Dec 7, 2016 •

edited

Loading

giuseppeg commented Dec 7, 2016 •

edited

Loading